Match type filters through re-export spellings via canonical code refs - #5800
Match type filters through re-export spellings via canonical code refs#5800lukemelia wants to merge 2 commits into
Conversation
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 1h 40m 0s ⏱️ - 14m 21s Results for commit 2a58eb1. ± Comparison against earlier commit 7921aea. Realm Server Test Results 1 files ±0 1 suites ±0 15m 22s ⏱️ +5s Results for commit 2a58eb1. ± Comparison against earlier commit 7921aea. |
A filter whose `type`/`on` ref named a type through a re-exporting module (e.g. file-api's FileDef, re-exported from card-api) silently matched nothing: index rows stamp `types` with the canonical (defining-module) key from identifyCard, so only the canonical spelling joined the membership keys. Filtering on base FileDef via its documented file-api module returned zero rows while subtype filters matched. The query engine now resolves each type condition's ref through the definition lookup and unions the canonical ref's spelling-tolerant keys with the as-given ref's, so both spellings compile to the same `types` membership predicates. The type condition becomes a deferred expression node (pass-1-resolved) since the definition lookup is async. The host's search resource applies the same rewrite before client-side matching — the matcher compares against identifyCard of the instance's class, so an uncanonicalized re-export spelling would strip the server's correct results during reconciliation. Until the rewrite settles for the active filter (or when a ref doesn't resolve), the search stays a server-only passthrough. The shared canonicalization walker single-flights duplicate ref spellings by memoizing the in-flight promise: sibling filter nodes are walked concurrently, and a value-memo would let both siblings miss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bcbbc1f to
7921aea
Compare
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review focus: the two-sided canonical-ref contract — that the server's types-membership compile and the host's client-side matcher agree on what a re-export spelling matches, and that the new deferred type-condition node composes exactly as the inline any() it replaces.
Bottom line: the design is correct — I found no correctness defect in the compiled SQL or in the server/matcher agreement. Two non-blocking findings: a stale-source race in the host's async canonicalization, and a swallow-all error path in typeKeysFor. Details inline. No blocking issues.
What lands right.
- The mechanism checks out end-to-end. Index rows stamp
typesviaidentifyCard(the defining-module ref —makeDefinitioninhost/app/routes/module.tssetscodeRef = identifyCard(cardOrFieldDef)). The matcher'sinstanceIsTypewalks the instance's prototype chain comparingidentifyCard(klass)throughcodeRefEquals, which tolerates URL-spelling variance (RRI / real-URL / virtual-alias) but not module-identity variance. So a rawfile-api/FileDeffilter makesinstanceIsTypereturn false and the reconciler strips the server's correct rows. Canonicalizing the filter's refs to thecard-apidefining-module ref on both sides is exactly the right fix, and doing it upstream insearch.tsrather than inside the matcher keeps that single call site as the only consumer needing it (verified:search.tsis the only non-test caller ofmatchInstanceAgainstFilter/isClientEvaluable). - The deferred
type-conditionnode is structurally equivalent to the old inline path.filterConditionstill wrapsthis.typeCondition(ref)(now[typeConditionNode]) throughevery/any, whoseaddExplicitParensparenthesizes the single-element array; pass 1 spliceshandleTypeCondition'sany(keys.map(typesContains))— the same token shape the old code emitted — inside those parens. All three type-key sites (handleTypeCondition,hasFileType,hasInstanceType) route throughtypeKeysFor, so no twin is left on the old inline path. typeKeysFormutatinginternalKeysFor's return with.pushis safe —internalKeysForreturns a fresh.map(...)array per call.- The canonicalization walker's single-flight memo is correct:
canonicalRefrunsmemo.has/memo.setsynchronously before its firstawait, so concurrent sibling walks can't both miss (theduplicate refs resolve oncetest pins it), and the per-node{ ...node }shallow clone keeps the input tree immutable (also pinned).
Recommendations.
- Guard the host
.thenagainst a stale filter so an out-of-orderloader.importresolution can't stickily disable client reconciliation — see theloadCanonicalizedFilterthread. (non-blocking) - Consider distinguishing "definition not found" from a transient lookup failure in
typeKeysFor, so a valid re-export ref doesn't silently under-match under load — see thetypeKeysForthread. (non-blocking)
Adjacent, out of scope. The realm test-results status comment reports realm-server tests as ±0 against the base even though this PR adds two tests to search-entries-engine-test.ts — most likely a status-comment/base-comparison artifact rather than the tests not running, but worth a glance to confirm they executed in CI.
| .then(({ filter: canonical, incomplete }) => { | ||
| if (isDestroyed(this) || isDestroying(this)) { | ||
| return; | ||
| } | ||
| this.canonicalizedFilter = { | ||
| source: filter, | ||
| filter: canonical, | ||
| incomplete, | ||
| }; |
There was a problem hiding this comment.
[Claude Code 🤖] Out-of-order canonicalization leaves a stale source and stickily disables client reconciliation (regression introduced by this PR; non-blocking — it degrades to a safe server-only passthrough, but silently and until the next query change).
The mechanism. loadCanonicalizedFilter runs on every live modify() (search.ts :424, before this.activeQuery = query at :426). Its early-return guard (this.canonicalizedFilter?.source === filter, :269) compares only against the settled value, so while a first canonicalization for filter A is still in flight, a second modify() carrying filter B (a new object) passes the guard and starts a second concurrent canonicalizeFilterRefs. This .then then writes canonicalizedFilter = { source: filter, … } unconditionally, so whichever promise settles last wins.
The two resolutions are loader.import(ref.module) calls: A triggers the cold import (possibly a network fetch); B, arriving later, resolves the same module from the loader cache almost immediately. So B settles first and sets source: B, then A settles and clobbers it with source: A. Now this.activeQuery.filter === B but this.canonicalizedFilter.source === A.
The consequence. isClientFilterEligible requires canonicalized.source === filter (:712–717), so a stale source makes it return false and displayedInstances returns the server set untouched (:589–593). Results stay correct, but live client-side reconciliation (candidate add + local no-match removal) stays off until the next modify() whose canonicalization happens to win the race — a live-refresh realm event calls this.search.perform(this.#previousQuery) without re-running modify(), so nothing re-triggers it in between. Sticky.
The fix. Drop a stale result in the .then. It fires synchronously after this.activeQuery = query is set within the same modify(), so activeQuery.filter is already the latest by the time any resolution lands:
| .then(({ filter: canonical, incomplete }) => { | |
| if (isDestroyed(this) || isDestroying(this)) { | |
| return; | |
| } | |
| this.canonicalizedFilter = { | |
| source: filter, | |
| filter: canonical, | |
| incomplete, | |
| }; | |
| .then(({ filter: canonical, incomplete }) => { | |
| if (isDestroyed(this) || isDestroying(this)) { | |
| return; | |
| } | |
| // A newer filter may have become active while this resolved; drop the | |
| // stale result so a late resolution can't clobber the current one. | |
| if (this.activeQuery?.filter !== filter) { | |
| return; | |
| } | |
| this.canonicalizedFilter = { | |
| source: filter, | |
| filter: canonical, | |
| incomplete, | |
| }; | |
| }) |
Related. Because the guard and memo key on filter object identity, an unstable () => query thunk that rebuilds a deeply-equal filter on each recompute re-triggers canonicalization (and a transient passthrough window) on every modify(), even though the querySignature deep-equal check at :537 already skips the redundant search. Keying eligibility off querySignature rather than identity would also close that gap — but the staleness guard above is the load-bearing fix.
Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 2a58eb1006. The .then now drops its result when this.activeQuery?.filter !== filter, so a late resolution (a cold loader.import settling after a cache-warm sibling) can no longer overwrite canonicalizedFilter with a source that points at a filter that is no longer active. The guard is sound because the .then fires only after this.activeQuery = query has run synchronously within the same modify(), so the comparison always sees the current filter.
The related identity-keying note (an unstable () => query thunk re-triggering canonicalization) is left as-is — the staleness guard makes the transient passthrough self-correct, and moving eligibility onto querySignature is a larger change than this finding warrants.
| } catch { | ||
| // fall through to the spelling-based keys | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] typeKeysFor swallows every lookupDefinition error, so a transient definition-lookup failure silently under-matches (follow-up / non-blocking; the not-found case matches prior behavior).
This catch {} treats all failures the same — fall back to spelling-only keys. For a genuinely nonexistent type that's correct and matches the pre-existing behavior (a bare { type: X } filter never validated existence). But lookupDefinition can also throw transiently — a prerender timeout or module-cache miss under load, a DB hiccup — and on a valid re-export ref (file-api's FileDef) that means the canonical (card-api) key never joins keys, so the query silently returns fewer rows (the canonical-stamped rows drop out) with no error surfaced. The engine raises FilterRefersToNonexistentTypeError for field-path lookups elsewhere; a type condition now carries the same async dependency but degrades silently instead.
Not this PR's blocker: the failure is transient and self-heals on retry, and the canonical-key expansion is what's new here rather than the whole type filter. Worth considering whether to distinguish "definition not found" (fall through, match nothing — fine) from an unexpected lookup error (surface it, as the field-path walk does). Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 2a58eb1006. The blanket catch {} is now catch (error) that re-throws unless isFilterRefersToNonexistentTypeError(error). A ref resolving to no known type still falls through to the spelling-based keys — matching how the engine already treats a nonexistent type as an empty result (the top-level _search catch), so the intentional spelling-key fallback and the transient-module-error behavior are unchanged. What changes is that a genuinely unexpected error (a bug, not a missing type) now propagates instead of being silently swallowed and narrowing the match.
Note the engine deliberately treats a FilterRefersToNonexistentType — which wraps transient module/prerender errors too — as match-nothing everywhere (field-path lookups included), so this keeps typeKeysFor consistent with that convention rather than special-casing transient failures here.
… errors Two robustness fixes from review of the canonical-code-ref type matching: - SearchResource.loadCanonicalizedFilter guards its `.then` against a stale filter. Sibling canonicalizations race and a cache-warm loader import can resolve ahead of a cold one, so an earlier filter's resolution could land last and overwrite `canonicalizedFilter` with a `source` no longer matching the active query. That left `isClientFilterEligible` false and the search stuck in server-only passthrough until the next query change. The guard drops a resolution whose filter is no longer the active one. - IndexQueryEngine.typeKeysFor no longer swallows every lookup error. A ref that resolves to no known type still falls through to the spelling-based keys (matching nothing unless rows were stamped under that spelling), matching how the engine's top-level catch treats a nonexistent type as an empty result; any other, unexpected error now propagates instead of silently narrowing the match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What this does
A
filter.type/oncode ref that names a type through a re-exporting module — the canonical example being baseFileDefaddressed via its documentedfile-apimodule, which re-exports it fromcard-api— silently matched nothing. Index rows stamptypeswith the canonical (defining-module) key fromidentifyCard, so only the canonical spelling ever joined the membership keys: filtering on baseFileDefreturned zero rows while concrete subtype filters matched, making the base-type filter a silent dead end.Two halves, kept in agreement:
typesmembership predicates union the canonical ref's spelling-tolerant keys (RRI / real-URL / virtual-alias) with the as-given ref's. Because the definition lookup is async, the type condition becomes a deferred expression node resolved in pass 1, alongside the existing deferred kinds. A ref whose definition doesn't resolve keeps only its spelling-based keys and matches nothing, exactly as before.identifyCard) before client-side matching — the matcher compares refs againstidentifyCardof an instance's class, so an uncanonicalized re-export spelling would strip the server's correct results during live reconciliation. Until the rewrite settles for the active filter, or when a ref doesn't resolve, the search stays a server-only passthrough so the two evaluations can't disagree.The shared walker (
runtime-common/query-canonicalization.ts) rewritestype/onrefs throughany/every/notwithout mutating the input tree, and single-flights duplicate ref spellings by memoizing the in-flight promise — sibling nodes are walked concurrently, so a value-memo would double-resolve.Test plan
packages/realm-server/tests/search-entries-engine-test.ts: a pure base-FileDef filter matches every file row across subtypes, and thefile-apire-export spelling matches the same rows as the canonicalcard-apispelling (36/36 pass locally).packages/host/tests/unit/query-canonicalization-test.ts: rewrite of top-level and nested refs, input-tree immutability, unresolvable refs marking the result incomplete, and duplicate refs resolving once (8/8 pass locally via the dev test page).🤖 Generated with Claude Code